New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Relative Path Images #9
Relative Path Images #9
Conversation
…does not exist in cwd (default behavour)
Very promising! Your solution is using some Eleventy specific code which is inevitable I think. A flag would ensure the default remains neutral. Still, the same logic would apply. I feel that the flag name would have to reflect its Eleventy nature. Maybe something like |
@solution-loisir Thanks for the feedback yesterday. I took some time to implement the flag as you suggested. I kept the reference to I added a test that builds the site a second time, with only the flag enabled, so the |
Oh-- and before it can be merged it would need a version bump to 0.8.2 (or something). :) |
Great! I had a quick peek and it looks very nice! I'll do a proper review over the weekend when I have time. I'll let you know if there's anything. Thank you so much for this! 😊 Since this is a new feature, version will be bumped to 0.9.0! |
Woot! Sounds like a plan. Looking forward to the feedback. 😊 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The more I think about this, the more I feel that it should be left to users to implement for their own use case instead of forcing opinions. So providing a callback instead of a flag that would give access to env
should do the trick. Something along this line:
if(!Image.Util.isRemoteUrl(normalizedTokenAttributes.src) && resolvePath) {
src = resolvePath(env);
}
Or maybe even better just pass env
to renderImage. I think this would be the most resilient in the ling run. DX could be dealt with by including examples in the docs (this PR would provide a good example for Eleventy!) Tell me what you think.
index.js
Outdated
@@ -25,7 +35,10 @@ module.exports = function markdownItEleventyImg(md, { | |||
|
|||
const normalizedTokenAttributes = generateAttrsObject(token).addContentTo("alt").attrs; | |||
|
|||
const src = normalizedTokenAttributes.src; | |||
let src = normalizedTokenAttributes.src; | |||
if(!Image.Util.isRemoteUrl(normalizedTokenAttributes.src) && eleventyResolveToProjectRoot == false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if(!Image.Util.isRemoteUrl(normalizedTokenAttributes.src) && eleventyResolveToProjectRoot == false) { | |
if(!Image.Util.isRemoteUrl(normalizedTokenAttributes.src) && env.page.inputPath && eleventyResolveToProjectRoot == false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking that env.page.inputPath
is not undefined. So we don't get an error when used outside Eleventy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes a lot of sense, especially combined with the callback you mentioned above. More specifically the resolvePath
callback. Thinking of it in my own usecase, I'm not sure how I would implement just a path resolution logic through the renderImage method.
That being said, if you know how it would work, then feel free to share. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yhea you're right, renderImage
would not let you implement just a path resolution logic. You would have to implement the full rendering logic as well. It's probably better to go with a resolvePath
callback, it's more straightforward. And in any case, renderImage
will receive the "proper" path anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking for env.page.inputPath
with the generic resolvePath
callback is likely not relevant anymore. As any code tied to a specific context would be implemented in the callback which receive env
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just pushed the resolvePath
change, complete with the env
variables. I also included the image path, although I'm unsure if that makes the most sense. Thoughts?
…e relative images
Alright-- updated version has been pushed. I added to our conversation about index.js above, but it feels a lot cleaner overall. I didn't include a typecheck on I did, however, include the Lastly-- I updated my relative image test to use the new Thoughts? |
That's great, I'm ready to merge! Thank you so much, it was a pleasure to work this out with you. This feature will be published under |
It was great working with you too! Thanks for being so responsive. Now to update my own 11ty project to use the latest version of this code. 😊 |
This is an incomplete PR that I put together to fix my own relative image issues for my 11ty project, BEFORE I noticed #8 where this is being discussed.
It's a bit of a hack, but I added some conditional logic when setting the
src
in the plugin, where if the file is not remote and does not exist in the default directory, it checks for is relative to the file it image comes from.I realize the desired behaviour is a config flag, but I thought I would share this as part of the discussion. Maybe it will help a bit to inspire some people.
Anyway-- I've marked it as draft so people can take a look. I've also added some test data, but they are failing. I'm hoping to take a closer look at it soon to figure out the best way to include tests around the new logic.
Feedback and discussion is appreciated. :)